Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Race Condition between the Creation of the S3 Bucket Policy and the CloudTrail Trail #91

Merged
merged 3 commits into from
Apr 15, 2024
Merged

Fix Race Condition between the Creation of the S3 Bucket Policy and the CloudTrail Trail #91

merged 3 commits into from
Apr 15, 2024

Conversation

X-Guardian
Copy link
Contributor

what

Fix the race condition between the creation of the S3 Bucket policy and the CloudTrail trail by adding a depends_on argument to the bucket_id output which is used as input to the CloudTrail module. This ensures that all the resources in the CloudTrail S3 Bucket module, including the S3 Bucket Policy have been created before the CloudTrail trail is created.

The example used for the tests has also been updated to include the creation of the CloudTrail Trail to verify that this is working.

why

@X-Guardian X-Guardian requested review from a team as code owners April 12, 2024 10:34
@mergify mergify bot added the triage Needs triage label Apr 12, 2024
@jamengual
Copy link
Contributor

/terratest

@jamengual
Copy link
Contributor

/terratest

@jamengual
Copy link
Contributor

What version of terraform are you using?
Usually, depends_on in outputs is not used, so I just want to make sure it is ok with other CP reviewers.

@X-Guardian
Copy link
Contributor Author

The Terraform version is not relevant. It is the AWS CloudTrail CreateTrail API that is checking the S3 Bucket Policy when creating the trail. Without the depends_on on the output this check happens before the S3 Bucket Policy has been applied.

jamengual
jamengual reviewed Apr 15, 2024
View reviewed changes
outputs.tf Outdated Show resolved Hide resolved
examples/complete/main.tf Show resolved Hide resolved
@Nuru
Copy link
Contributor

Nuru commented Apr 15, 2024

/terratest

@Nuru Nuru added bugfix Change that restores intended behavior and removed triage Needs triage labels Apr 15, 2024
Nuru
Nuru approved these changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am uncomfortable allowing the use of an undocumented feature, and this should probably be fixed upstream in terraform-aws-s3-bucket, but it works with Terraform 1.3, 1.5, and 1.8 and I don't see a better way to achieve the desired result, so I will allow this and we can see how it goes.

@Nuru Nuru merged commit 91595d9 into cloudposse:main Apr 15, 2024
27 checks passed
@X-Guardian
Copy link
Contributor Author

X-Guardian commented Apr 15, 2024 •
edited
Loading

Thank for merging this @Nuru. If you are talking about the depends_on feature for an entire module being undocumented, it is documented here: https://developer.hashicorp.com/terraform/language/meta-arguments/depends_on.

When the dependency object is an entire module, depends_on affects the order in which Terraform processes all of the resources and data sources associated with that module.

And depends_on for outputs is documented here: https://developer.hashicorp.com/terraform/language/values/outputs#depends_on-explicit-output-dependencies

@X-Guardian X-Guardian deleted the bucket-policy-race-condition-fix branch April 15, 2024 21:56
@andreineculau
Copy link

andreineculau commented Jul 25, 2024 •
edited
Loading

Weird, but I have just experienced this, despite this fix.

I take it back. The cloudtrail module was missing a depends_on cloudtrail_s3_bucket, due to s3_bucket_name being static.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamengual jamengual jamengual left review comments

@Nuru Nuru Nuru approved these changes

@hans-d hans-d Awaiting requested review from hans-d hans-d is a code owner automatically assigned from cloudposse/contributors

@johncblandii johncblandii Awaiting requested review from johncblandii johncblandii is a code owner automatically assigned from cloudposse/contributors

Assignees
No one assigned
Labels
bugfix Change that restores intended behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Race Condition between the Creation of the S3 Bucket Policy and the CloudTrail Trail
4 participants
@X-Guardian @jamengual @Nuru @andreineculau